-
Notifications
You must be signed in to change notification settings - Fork 106
feat(AggLayer): B2AGG note consumption check
#2334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: agglayer
Are you sure you want to change the base?
Conversation
- make bridge a network account - move the ID check to non-reclaim branch
69ed247 to
bec68d1
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left some comments inline. Also, I think it may make sense to merge this PR after #2338.
| const ERR_B2AGG_TARGET_ACCOUNT_MISMATCH="B2AGG note attachment target account does not match consuming account" | ||
|
|
||
|
|
||
| #! Bridge-to-AggLayer (B2AGG) note script: bridges assets from Miden to an AggLayer-connected chain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's add NOTE SCRIPT section header above this line.
| #! - If the consuming account is the sender (reclaim): the note's assets are added back to the consuming account. | ||
| #! - If the consuming account is the Agglayer Bridge: the note's assets are moved to a BURN note, | ||
| #! and the note details are hashed into a leaf and appended to the Local Exit Tree. | ||
| #! global exit root (GER) merkle tree structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement seems to be dangling (and not sure if it is relevant here).
| # Ensure note attachment targets the consuming bridge account. | ||
| exec.active_note::get_metadata | ||
| # => [NOTE_ATTACHMENT, METADATA_HEADER, pad(8)] | ||
|
|
||
| # TODO simplify once https://github.com/0xMiden/miden-base/pull/2338 lands | ||
|
|
||
| swapw dropw | ||
| # => [NOTE_ATTACHMENT, pad(12)] | ||
|
|
||
| # Reorder attachment word to [target_id_prefix, target_id_suffix]. | ||
| drop drop | ||
| # => [target_id_prefix, target_id_suffix, pad(14)] (auto-padding) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const ERR_UPDATE_GER_UNEXPECTED_NUMBER_OF_STORAGE_ITEMS = "UPDATE_GER script expects exactly 8 note storage items" | ||
| const ERR_UPDATE_GER_TARGET_ACCOUNT_MISMATCH = "UPDATE_GER note attachment target account does not match consuming account" | ||
|
|
||
| #! Agglayer Bridge UPDATE_GER script: updates the GER by calling the bridge_in::update_ger function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's add NOTE SCRIPT section header above this line.
| /// Contains the destination network and address information required | ||
| /// for bridging assets to the AggLayer network. | ||
| #[derive(Debug, Clone)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comment wrapping.
| /// Destination network identifier (AggLayer-assigned network ID) | ||
| pub destination_network: u32, | ||
| /// Destination Ethereum address (20 bytes) | ||
| pub destination_address: EthAddressFormat, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (and not related to this PR): I think we should call it just EthAddress rather than EthAddressFormat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to be call EthAddress, but was renamed as per #2238 (comment)
However, this was in the light of bridging-in, where we were passing a 160 bit value that was represented as an address on Ethereum, but was in fact encoding a Miden AccountId - i.e. it was not a "valid" Ethereum address ("valid" in the sense that there was no account associated with it).
In the bridging out flow, the value carried by the B2AGG note will actually be a valid Ethereum destination address.
I wouldn't change the naming to EthAddress (because it is not strictly correct) across the board.
I think the best solution here is to:
- create a separate
EthAddressdedicated for the bridging-out flow. It also wouldn't have methods such asfrom_account_id(account_id: AccountId)as they don't make sense there. - keep
EthAddressas the base struct, and create aEthAddressFormattrait to encompass Miden<>Ethereum conversion logic likefrom_account_id/from_account_id
WDYT?
| /// # Parameters | ||
| /// - `storage`: The destination network and address information | ||
| /// - `assets`: The assets to bridge (must be fungible assets from a network faucet) | ||
| /// - `target_account_id`: The account ID that will consume this note (bridge account) | ||
| /// - `sender_account_id`: The account ID of the note creator | ||
| /// - `note_type`: The type of note (Public or Private) | ||
| /// - `rng`: Random number generator for creating the note serial number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need note_type here? I think B2AGG notes would have to always be public (otherwise, the bridge can't process them).
| let tag = NoteTag::new(0); | ||
|
|
||
| let attachment = NoteAttachment::from( | ||
| NetworkAccountTarget::new(target_account_id, NoteExecutionHint::None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use NoteExecutionHint::Always here because B2AGG notes are always consumable.
|
|
||
| let bridge_out_component = bridge_out_component(vec![]); | ||
| // Create the "bridge_out" component | ||
| let let_storage_slot_name = StorageSlotName::new("miden::agglayer::let").unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not for this PR, but we should normalize storage slot naming. For example, this should be miden::agglayer::bridge::let to be consistent with miden::agglayer::bridge::ger_upper and miden::agglayer::bridge::ger_lower (which I think should be just one slot miden::agglayer::bridge::ger).
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me (only reviewed the note attachment parts - let me know if you want me to review everything).
| /// | ||
| /// # Errors | ||
| /// Returns an error if note creation fails. | ||
| pub fn create_b2agg_note<R: FeltRng>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: For future note creation functions, I think we should directly implement the approach described in #2283, since we'll have to migrate all standard note types to this approach anyway.
| let b2agg_script = b2agg_script(); | ||
| let recipient = NoteRecipient::new( | ||
| rng.draw_word(), | ||
| miden_protocol::note::NoteScript::new(b2agg_script), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| miden_protocol::note::NoteScript::new(b2agg_script), | |
| NoteScript::new(b2agg_script), |
Nit
As per the discussion here, the network target account ID is placed into
NoteAttachmentinstead.While at it, I created a helper function, similarly to the one we have for
CLAIMnotes:create_b2agg_note, and refactored the tests to use that utility.TODO:
UPDATE_GERnote (once feat(AggLayer):UPDATE_GERnote #2333 lands)NoteAttachmentSchemeshould be used?closes #2173
closes #2189